Attachment package refactor #311

dean-journeyapps · 2025-07-22T12:00:55Z

No description provided.

packages/powersync_attachments_stream/pubspec.yaml

packages/powersync_attachments_stream/lib/src/utils/mutex.dart

packages/powersync_attachments_stream/lib/src/local_storage.dart

packages/powersync_attachments_stream/lib/src/attachment_service.dart

simolus3

Some more comments from a quick look.

Also, I assume supabase-todolist-new-attachments was copied from the old one? For a review it would be easier if the changes were in the same project so I can see what has changed. But that can happen in the end.

packages/powersync_attachments_stream/lib/powersync_attachments_stream.dart

packages/powersync_attachments_stream/lib/src/attachment.dart

packages/powersync_attachments_stream/lib/src/attachment_queue_service.dart

packages/powersync_attachments_stream/test/local_storage_test.dart

simolus3

A few more comments, most of them pretty minor again :)

packages/powersync_attachments_stream/analysis_options.yaml

packages/powersync_attachments_stream/lib/src/abstractions/local_storage.dart

packages/powersync_attachments_stream/lib/src/attachment_queue_service.dart

packages/powersync_attachments_stream/lib/src/implementations/attachment_context.dart

packages/powersync_attachments_stream/lib/src/implementations/attachment_service.dart

simolus3 · 2025-08-12T08:16:53Z

packages/powersync_attachments_stream/lib/src/storage/io_local_storage.dart

+import 'dart:async';
+import 'dart:io';
+import 'dart:typed_data';
+import 'package:path/path.dart' as p;
+import '../abstractions/local_storage.dart';


nit: Typically one inserts an empty line between dart: and package: imports and then another one before local imports. All the effective Dart samples do that, but it's not spelled out explicitly. I've added a suggestion for this here, it affects other files as well.

Suggested change

import 'dart:async';

import 'dart:io';

import 'dart:typed_data';

import 'package:path/path.dart' as p;

import '../abstractions/local_storage.dart';

import 'dart:async';

import 'dart:io';

import 'dart:typed_data';

import 'package:path/path.dart' as p;

import '../abstractions/local_storage.dart';

simolus3 · 2025-08-12T08:17:46Z

packages/powersync_attachments_stream/lib/src/attachment_queue_service.dart

+                  attachment.copyWith(state: AttachmentState.archived),
+                );
+              }
+              break;


You don't need this break anymore in recent Dart versions.

packages/powersync_attachments_stream/lib/src/storage/io_local_storage.dart

simolus3 · 2025-08-12T08:25:47Z

packages/powersync_attachments_stream/lib/src/sync/syncing_service.dart

+      handleData: (data, sink) {
+        sink.add(data);
+        // Simple throttle implementation - just delay the next event
+        Future.delayed(throttle);


This future is unawaited and will not actually throttle anything.

I think it would be easier to not throttle the stream here and instead add a duration parameter to watchActiveAttachments. We can forward that to db.watch() instead of duplicating the throttle implementation here. That means we wouldn't be throttling manualTriggers anymore, but I think that's fine.

packages/powersync_attachments_stream/lib/src/attachment_queue_service.dart

simolus3

The implementation looks good now, I have a few nits on types. I also think we should strive to not introduce classes / interfaces we don't strictly need, especially if they're publicly exported. Also more generally:

Please run dart format on the package. I also recommend enabling format on save.
I think a few more tests with an actual database would be nice to have, you can take a look at the Kotlin tests for attachments for inspiration. We can test on native platforms only in the beginning, you can probably copy something like this to set up databases. But it's still a bit tricky, let me know if you need help with this. I can also push a basic scaffold for tests to this branch.

simolus3 · 2025-08-13T13:35:01Z

packages/powersync_attachments_stream/.metadata

@@ -0,0 +1,10 @@
+# This file tracks properties of this Flutter project.


Given that this package is no longer a Flutter project, I think we can delete this file.

simolus3 · 2025-08-13T16:00:08Z

packages/powersync_attachments_stream/lib/powersync_attachments_stream.dart

+/// Default exports for native platforms (dart:io). For web, use 'common.dart'.
+export 'common.dart';
+export 'src/storage/io_local_storage.dart';
+export 'src/attachment_queue_service.dart';


This is exported by common.dart now and can be removed here.

simolus3 · 2025-08-13T16:02:44Z

packages/powersync_attachments_stream/lib/src/abstractions/attachment_context.dart

+  ///
+  /// [id]: The ID of the attachment to delete.
+  /// [tx]: The database context to use for the operation.
+  Future<void> deleteAttachment(String id, dynamic context);


This shouldn't be dynamic, can this be a SqliteWriteContext?

simolus3 · 2025-08-13T16:03:10Z

packages/powersync_attachments_stream/lib/src/abstractions/attachment_context.dart

+  ///
+  /// [id]: The ID of the attachment to delete.
+  /// [tx]: The database context to use for the operation.
+  Future<void> deleteAttachment(String id, dynamic context);


Also this function isn't called at all, I assume that should happen in SyncingService.deleteAttachment?

simolus3 · 2025-08-13T16:03:31Z

packages/powersync_attachments_stream/lib/src/abstractions/attachment_context.dart

+  /// [attachment]: The attachment to upsert.
+  /// [context]: The database transaction/context to use for the operation.
+  /// Returns the upserted [Attachment].
+  Future<Attachment> upsertAttachment(Attachment attachment, dynamic context);


context should likely be a SqliteWriteContext here as well?

simolus3 · 2025-08-13T16:09:40Z

packages/powersync_attachments_stream/lib/src/abstractions/local_storage.dart

@@ -0,0 +1,34 @@
+import 'dart:typed_data';
+
+abstract class AbstractLocalStorageAdapter {


Let's make this an interface class and just name it LocalStorageAdapter:

Suggested change

abstract class AbstractLocalStorageAdapter {

abstract interface class LocalStorageAdapter {

simolus3 · 2025-08-13T16:11:24Z

packages/powersync_attachments_stream/lib/src/attachment_queue_service.dart

+    return fileExtension != null
+        ? '$attachmentId.$fileExtension'
+        : '$attachmentId.dat';


Suggested change

return fileExtension != null

? '$attachmentId.$fileExtension'

: '$attachmentId.dat';

return '$attachmentId.${fileExtension ?? 'dat'}';

simolus3 · 2025-08-13T16:13:21Z

packages/powersync_attachments_stream/lib/src/attachment_queue_service.dart

+            .cast<Attachment?>()
+            .firstWhere(
+              (a) => a != null && a.id == item.id,
+              orElse: () => null,
+            );


I think a more intuitive way to express this without the cast() may be

Suggested change

.cast<Attachment?>()

.firstWhere(

(a) => a != null && a.id == item.id,

orElse: () => null,

);

.where((a) => a.id == item.id)

.firstOrNull;

simolus3 · 2025-08-13T16:14:44Z

packages/powersync_attachments_stream/lib/src/attachment_queue_service.dart

+    required String mediaType,
+    String? fileExtension,
+    String? metaData,
+    required Future<void> Function(dynamic context, Attachment attachment)


Suggested change

required Future<void> Function(dynamic context, Attachment attachment)

required Future<void> Function(SqliteWriteContext context, Attachment attachment)

simolus3 · 2025-08-13T16:14:57Z

packages/powersync_attachments_stream/lib/src/attachment_queue_service.dart

+  /// The default implementation assumes the attachment record already exists locally.
+  Future<Attachment> deleteFile({
+    required String attachmentId,
+    required Future<void> Function(dynamic context, Attachment attachment)


Suggested change

required Future<void> Function(dynamic context, Attachment attachment)

required Future<void> Function(SqliteWriteContext context, Attachment attachment)

stevensJourney · 2025-08-14T06:29:37Z

demos/supabase-todolist/lib/widgets/todo_item_widget.dart

+      await attachmentQueue.deleteFile(
+        attachmentId: todo.photoId!,
+        updateHook: (context, attachment) async {
+          // await context.execute("UPDATE todos SET photo_id = NULL WHERE id = ?", [todo.id]);


I think you should probably still do the un-assignment here, is uncommenting this perhaps a TODO?

demos/supabase-todolist/pubspec.yaml

…handling in powersync_attachments_stream package and added unit tests

…/powersync.dart into attachment-package-refactor

…_stream package. Removed unused imports, improved error handling during file uploads and downloads, and enhanced and renamed logging for better traceability of attachment operations.

…hods, improving clarity on attachment lifecycle management, state handling, and local storage operations.

…hments package implementation to supabase-todolist demo.

… path for local storage in the powersync_attachments_stream package. Simplified the attachment watching logic. Updated README for clarity on schema and attachment states.

… classes to utilize the new interface and adjusted README for consistency.

… package. Updated attachment queue initialization to use localStorage, and enhanced README for clarity on storage handling. Added tests for edge cases and robustness in local storage operations.

…consistency. Removed outdated comment regarding attachments directory in attachment queue service documentation.

…dated photo capture widget to save attachments as byte data instead of streams, and modified related methods in the attachment queue service to accept byte data. Updated tests to reflect changes in data handling.

…comments, replacing multiple error handler parameters with a single optional error handler for sync-related errors.

…fixed various formatting

@experimental

…all new classes as @experimental

dean-journeyapps requested a review from simolus3 July 22, 2025 12:00

simolus3 reviewed Jul 22, 2025

View reviewed changes

simolus3 reviewed Jul 30, 2025

View reviewed changes

simolus3 reviewed Aug 12, 2025

View reviewed changes

simolus3 reviewed Aug 13, 2025

View reviewed changes

stevensJourney reviewed Aug 14, 2025

View reviewed changes

simolus3 mentioned this pull request Aug 14, 2025

Relax fileExtension requirement in attachmentQueue #269

Open

dean-journeyapps and others added 20 commits August 19, 2025 15:38

initial boilerplate of new attachments package

37f9017

Update dependencies and enhance LocalStorage interface with metadata …

dc9952c

…handling in powersync_attachments_stream package and added unit tests

init attachments package stream and demo with implementation

8846e72

Merge branch 'attachment-package-refactor' of github.com:powersync-ja…

d1fc46d

…/powersync.dart into attachment-package-refactor

Refactor attachment handling and logging in the powersync_attachments…

8848f70

…_stream package. Removed unused imports, improved error handling during file uploads and downloads, and enhanced and renamed logging for better traceability of attachment operations.

Added comprehensive comments and descriptions for key classes and met…

c4a8a77

…hods, improving clarity on attachment lifecycle management, state handling, and local storage operations.

Removed supabase-todolist-new-attachment demo and added the new atatc…

d76bcc2

…hments package implementation to supabase-todolist demo.

Refactor attachment queue initialization and implemented default base…

e747c76

… path for local storage in the powersync_attachments_stream package. Simplified the attachment watching logic. Updated README for clarity on schema and attachment states.

Removed redundant logger names.

be8d4af

Renamed SyncErrorHandler to AbstractSyncErrorHandler. Updated related…

1d2c32e

… classes to utilize the new interface and adjusted README for consistency.

Refactor local storage implementation in powersync_attachments_stream…

4d08952

… package. Updated attachment queue initialization to use localStorage, and enhanced README for clarity on storage handling. Added tests for edge cases and robustness in local storage operations.

Refactor logger variable name in attachment queue initialization for …

934eb9c

…consistency. Removed outdated comment regarding attachments directory in attachment queue service documentation.

Removed unecessary comment

320fab4

Added attachment queue service as an export in common.dart

525700c

Updated documentation in SyncingService to streamline error handling …

5ca3917

…comments, replacing multiple error handler parameters with a single optional error handler for sync-related errors.

Remoed dynamic variables, implemented context.deleteAttachment() and …

9ec641e

…fixed various formatting

Formatting fixes

f788773

Moved attachments stream implementation to powersync_core and marked …

bd26d9e

…all new classes as @experimental

Reduce public interface

248d748

simolus3 force-pushed the attachment-package-refactor branch from fa2f16e to 248d748 Compare August 19, 2025 14:36

simolus3 added 2 commits August 19, 2025 17:21

Rename library

a3ea19d

Fix local storage tests

d7daefc

Unit tests for attachments

57a4214

		@@ -0,0 +1,10 @@
		# This file tracks properties of this Flutter project.

		@@ -0,0 +1,34 @@
		import 'dart:typed_data';

		abstract class AbstractLocalStorageAdapter {

	abstract class AbstractLocalStorageAdapter {
	abstract interface class LocalStorageAdapter {

	required Future<void> Function(dynamic context, Attachment attachment)
	required Future<void> Function(SqliteWriteContext context, Attachment attachment)

Attachment package refactor #311

Are you sure you want to change the base?

Attachment package refactor #311

Conversation

dean-journeyapps commented Jul 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simolus3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simolus3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simolus3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!